-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PEK 850: Adding part of the higher priority tests #165
Conversation
tests/tests/test_guest_cpu_off.py
Outdated
# turn off arbitrary cpus | ||
cpu_on_off('/sys/devices/system/cpu/cpu17/online', 0) | ||
cpu_on_off('/sys/devices/system/cpu/cpu60/online', 0) | ||
cpu_on_off('/sys/devices/system/cpu/cpu109/online', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cpu number can depend on the platform we are testing; using static numbers can be a problem
tests/tests/test_guest_cpu_off.py
Outdated
m = Qemu.QemuSSH(qm) | ||
|
||
# turn off arbitrary cpus | ||
cpu_on_off('/sys/devices/system/cpu/cpu17/online', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it is good to turn on those cores before running the guest to make sure turning of them here might be impactful for the guest
tests/tests/test_guest_cpu_off.py
Outdated
qm.stop() | ||
|
||
# turn back on cpus | ||
cpu_on_off('/sys/devices/system/cpu/cpu17/online', 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would bring on the cores as soon as possible when i m done to avoid situations where there is an exception in the test (here it will be at the guest stop) and we do not bring them back on
tests/tests/test_guest_cpu_off.py
Outdated
qm = Qemu.QemuMachine() | ||
qm.run() | ||
|
||
# get pid of Qemu machine (not written immediately sometimes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would abstract this feature in the Qemu.py as a service, we can have a check_run function that makes sure that the qemu process gets started and returns the pid of the qemu process; this will allow to simplify the tests reading. The idea is to make the test case as "human" friendly as possible
tests/tests/test_guest_cpu_off.py
Outdated
m = Qemu.QemuSSH(qm) | ||
|
||
# pin pid to a particular cpu | ||
cs = subprocess.run(['sudo', 'taskset', '-pc', '18', f'{pid}'], capture_output=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again; using the static cpu number here can be a problem, if we do not want a complex test; we can skip the test if the number of core is less than 64 and we will use static numbers less than 64 in our tests and we can be sure that those numbers are valid
tests/tests/test_guest_fail.py
Outdated
assert cs.returncode == 0, 'Failed getting dmesg' | ||
dmesg_start = str(cs.stdout) | ||
|
||
# Setup ept=0 in driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this as a function in util.py; load_kvm_intel() for example
tests/tests/test_guest_fail.py
Outdated
dmesg_start = str(cs.stdout) | ||
|
||
# Setup ept=0 in driver | ||
cs = subprocess.run(['sudo', 'rmmod', 'kvm_intel'], check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rmmod will fail if there is any VM running, rmmod: ERROR: Module kvm_intel is in use
; i dont know how we can handle this situation
bd31fc9
to
799991c
Compare
tests/tests/setup-env-tox.sh
Outdated
python3-cpuinfo &> /dev/null | ||
# Make sure kvm_intel is installed properly | ||
# (one of the tests installed it with NOEPT) | ||
sudo rmmod kvm_intel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spmcmillan what is the reason to reload kvm_intel here ? i think kvm_intel is part of the prerequisite of the testing (like kernel, qemu installation). Moreover, modprobe kvm_intel might not be enough, we should provide the argument tdx=1 : modprobe kvm_intel tdx=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says in the comment, but we load it differently at times on the different tests. So, this is to make sure we load it the same way at the beginning of each test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally, each test should reload the kernel module properly if it previously unloaded it, but in practice, if there is any reason you cannot have this certainty, loading it in the tox global setup will not be enough, we have to make sure to do it at the beginning of each test. So i think, for now, i will go for the ideal situation where each test should unload/reload the kernel module properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true and agreed
799991c
to
839dbba
Compare
have a better cpu selection mechanism
add also kvm_intel module manager (as context manager for better robustness)
use the kvm_module context manager fix the qemu failure check
+ improve test check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I didn't approve yet because of the init command pending item, but will if that is ok
839dbba
to
5109e86
Compare
Adding tests to tdx set. Still need to add to checkbox tests in a separate PR with the rest of the tests specified in PEK850 in next pulse.
Fixes PEK-850